Add reload-failed-proxied-images feature
#2393
Conversation
I don't like you TravisCI... these last changes where not necessary...
... Travis... go home... you are drunk...
| // Handle failure of loading the image | ||
| // (maybe, the original image source failed to provide the image to the proxy in the 4 seconds timeframe) | ||
| tester.addEventListener('error', () => { | ||
| let tries = parseInt(image.dataset.brokenProxiedImage || '0', 10); |
There was a problem hiding this comment.
We can pass tries into the test function and avoid updating the DOM.
There was a problem hiding this comment.
I am not sure if there is a case where init will be fired (by the onAjaxedPages) while the page will still have the same images.
I get your point but I think the problem is both traffic and cache related. So since the client already made the 1st request, the badges server has already started creating and caching your image, an additional request will not cause that much trouble assuming that the image is cached and ready to be served. Thus we can for example allow only 1 or 2 tries with a bigger delay (maybe 5 seconds). We can also ask the developers over at https://github.com/badges/shields if this will cause additional problems. Of course, as you said, the point is not to make the problem worse. |
|
@fregante This problem is not limited to badges (based on my experiences). |
| test(image); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
an additional request will not cause that much trouble assuming that the image is cached and ready to be served.
Let's retry once, then.
If you use https://github.com/fregante/image-promise this whole feature can just be:
async function handleErroredImage({target}): Promise<void> {
await delay(5000);
try {
// A cloned image retries downloading
// `loadImage` awaits it
// If successfully loaded, the failed image will be replaced.
target.replaceWith(await loadImage(target.cloneNode()));
} catch {}
}
function init(): void {
delegate('img[src^="https://camo.githubusercontent.com"]', 'error', handleErroredImage);
}
features.add({
id: __featureName__,
description: 'Retries downloading images that failed downloading due to GitHub limited proxying.',
screenshot: false,
init
});There was a problem hiding this comment.
Based on my tests/debugging delegate does not work in this case. I think GitHub gabs the errors for statistics and the event does not bubble up to the body element. Maybe I am wrong.
There was a problem hiding this comment.
It works if you pass a true as the last parameter of delegate to make it a capture listener.
fregante
changed the title
reload-failed-proxied-images feature
|
Thank you ^_^ |
|
Hi from Shields! I noticed this was mentioned from badges/shields#1568. I'll give my belated 👍on this as the general alternative is reloading the entire readme which typically forces a reload of all the images. It's both a better experience and overall less traffic to reload just the broken images. Can I ask whether this reloads the images just once, or if it repeatedly attempts to reload the images? I'd favor the former, or else it'd be a good idea to include some kind of backoff. By the way, we're tracking the current capacity problem at badges/shields#3874 and hope to have some resolution shortly! |
|
@paulmelnikow Due to various traffic concerns, it reloads the failed images 5 seconds after the error event, only once. |
Description
Many times the badges images fail to load. This feature detects such failed to load images and tries to reload them so that the user don't have to manually reload the hole page.
Test
On the issue badges/shields#1568 there is a 4 seconds delayed badge that will always fail to load (the GitHub's proxy timeouts) you can see on the console tab that the feature will try to reload that image up to 6 times.
Preview